add plugin client factory #71

fbourigault · 2017-05-11T20:14:43Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	partially php-http/HttplugBundle#109
Documentation	TODO
License	MIT

This is required to display plugins created by 3rd party libraries in the HttplugBundle profiler. See php-http/HttplugBundle#109 (comment) for all puzzle pieces.

To Do

Update the changelog.
Update the documentation.

Nyholm

The option parameter is arbitrary, there is no way to make plugin clients be interchangeable. Should we define the options? Do we need options at all in the plugin client factory?

Nyholm · 2017-05-12T05:10:15Z

src/PluginClientFactory.php

+namespace Http\Client\Common;
+
+/**
+ * Create a PluginClient.


We could have a more expressive comment.

Or maybe remove this class comment and only use the one on the interface.

Nyholm · 2017-05-12T05:10:35Z

src/PluginClientFactoryInterface.php

+use Http\Client\HttpClient;
+
+/**
+ * Create a PluginClient.


Same here. :)

I'm open to any suggestion :)

Maybe:

Create a PluginClient object loaded with an array of plugins. A factory may add or modify the plugins or change the HttpClient.

Nyholm · 2017-05-12T05:12:41Z

src/PluginClientFactoryInterface.php

+     * @param Plugin[]                   $plugins
+     * @param array                      $options
+     *
+     * @see PluginClient constructor for $options details.


No, we cannot "see the PluginClient". We are not aware of any client implementation. We should write something like "array of options that are passed to the client."

I don't think so. There is only one PluginClient without a dedicated interface and this implementation is final. There is no way to write an other PluginClient!

You are correct. I had in mind that one could create ANY PluginClient with this. But the PluginClientFactoryInterface creates an instance of the concrete class PluginClient. That is a huge different from our other *FactoryInterfaces. They create an instance of an interface.

Nyholm · 2017-05-12T06:53:14Z

Note:
The PluginClientFactoryInterface creates an instance of the concrete class PluginClient. That is a huge different from our other *FactoryInterfaces. They create an instance of an interface.

sagikazarmark · 2017-05-12T08:37:44Z

TBH, I don't really see the point of this interface.

sagikazarmark · 2017-05-12T08:38:49Z

I don't see any alternative implementation options, so why an interface?

Creating a plugin client is configuration which is always specific to the application, so why standardizing it?

fbourigault · 2017-05-12T08:42:22Z

The idea behind is to use discovery to find a concrete PluginClientFactory. When the bundle profiling is enabled, a profile aware PluginClientFactory is decorates each plugin with a ProfilePlugin and add the StackPlugin at the begining. See php-http/HttplugBundle#109 (comment) for a whole plan details.

sagikazarmark · 2017-05-12T08:46:58Z

Why do we need a factory to decorate plugins? And why do we have to discover it? I still think it's configuration: when profiling is enabled, hook into DI and wrap plugins, otherwise don't.

fbourigault · 2017-05-12T08:50:08Z

It's all about displaying plugins from 3rd party like php-github-api in the profiler. With such implementation, you can integrate any 3rd party library (which use this interface and the discovery) in the profiler without having any specific code in the 3rd party library, without having a glue bundle and without having to write a line of configuration!

fbourigault · 2017-05-12T22:31:56Z

I updated the PR to add a note in the changelog. I also adressed @Nyholm comments about docblocks. I added in my to do list the puli stuff. Can someone help me with it?

EDIT: I think I found what I have to do with puli. Does I need to add some tests related to puli?

fbourigault · 2017-05-16T05:58:13Z

How can I test puli resources declaration?

fbourigault · 2017-06-15T16:00:00Z

This is now updated to provide a solution which does not require php-http/discovery to profile 3rd party library clients.

Nyholm

I understand better now why we use the callable. We loose a lot of code and interfaces.

Nyholm · 2017-06-15T16:02:32Z

src/PluginClientFactory.php

+     * Set the factory to use. The callable to provide must have the same arguments and return type as
+     * PluginClientFactory::createClient.
+     *
+     * @internal


Maybe state why?

This is used by the HTTPlugBundle to provide a better Symfony integration

Nyholm · 2017-06-15T16:04:11Z

src/PluginClientFactory.php

+     */
+    public function createClient($client, array $plugins = [], array $options = [])
+    {
+        $factory = static::$factory ?: function ($client, array $plugins, array $options) {


Why not simply do:

if (static::$factory) { return $factory($client, $plugins, $options); } return new PluginClient($client, $plugins, $options);

It would be easier to read.

fbourigault · 2017-06-15T16:12:52Z

Thank you @Nyholm. I addressed all your remarks.

Nyholm

Good, just fix the travis error

Nyholm · 2017-06-15T17:19:49Z

src/PluginClientFactory.php

+    public function createClient($client, array $plugins = [], array $options = [])
+    {
+        if (static::$factory) {
+            return (static::$factory)($client, $plugins, $options);


This is not allowed in PHP 5.x. ( I just found out)

$func = static::$factory; return $func($client, $plugins, $options); // Or: (This might work) return static::$factory($client, $plugins, $options);

fbourigault · 2017-06-15T20:52:07Z

Should be fixed now. Please, don't merge until it's ok on other packages.

Nyholm · 2017-07-01T16:45:27Z

@fbourigault Is this ready to merge?

Do @dbu or @sagikazarmark want to have a final review?

sagikazarmark

I understand why we need this, but I am still not a huge fan.

sagikazarmark · 2017-07-01T20:54:36Z

src/PluginClientFactory.php

+    /**
+     * @var callable
+     */
+    private static $factory;


Is this the workaround for not having an interface? If so, having an interface for this is still less evil. This solution provides zero type safety (even if the support PHP versions do not support return types).

As the stuff is marked internal, we choosed to avoid over engineering and to not use an interface.

fbourigault · 2017-07-03T14:18:21Z

I added the client_name option.

Nyholm · 2017-07-06T07:52:53Z

I will merge this, but we should review it a final time before releasing.

Thank you for the comments and thank you for the work on this PR

dbu · 2017-07-10T06:13:16Z

src/PluginClientFactory.php

+        if (static::$factory) {
+            $factory = static::$factory;
+
+            return $factory($client, $plugins, $options);


deos simply return static::$factory(...) not work?

Only in PHP 7+

weird. can you add a comment that this is to work with php 5 and can be removed in 7?

I was wrong. It doesn't work in any PHP version: "Function name must be a string".
See https://3v4l.org/WA5ek.

alright. then maybe a comment that using it directly does not work at least up to php 7.1?

Doesn't work too in 7.2.0alpha2. IMHO it doesn't worth to write a comment about something not supported by the language itself.

dbu · 2017-07-10T06:15:40Z

src/PluginClientFactory.php

+     *
+     * @return PluginClient
+     */
+    public function createClient($client, array $plugins = [], array $options = [])


why is this method dynamic but the factory callable static? this means i must instantiate the factory, but i can only set one single callable. why not make the callable an optional constructor argument?

The setFactory is static as we need to change the implementation at runtime when bundle profiling is enabled and, as we aim to do 0config integration, we can't have something non-static here.

ah, i see. maybe a bit of comment in the code would help so we remember in the future ;-)

dbu · 2017-07-10T06:16:59Z

could we use the class docblock to explain why this factory should be used?

Nyholm reviewed May 12, 2017

View reviewed changes

fbourigault mentioned this pull request May 23, 2017

add plugin client factory discovery php-http/discovery#94

Closed

3 tasks

fbourigault mentioned this pull request Jun 15, 2017

Show plugins created without the Symfony config php-http/HttplugBundle#109

Closed

Nyholm reviewed Jun 15, 2017

View reviewed changes

Nyholm approved these changes Jun 15, 2017

View reviewed changes

sagikazarmark reviewed Jul 1, 2017

View reviewed changes

add plugin client factory

a9a52c3

Nyholm merged commit fd4fd0a into php-http:master Jul 6, 2017

Nyholm mentioned this pull request Jul 6, 2017

Release 1.6 #81

Closed

dbu reviewed Jul 10, 2017

View reviewed changes

This was referenced Jul 10, 2017

add PluginClientFactory comments #83

Merged

Do not pass client_name option to PluginClient constructor #84

Merged

Added PluginClientBuilder #40

Closed

add plugin client factory #71

add plugin client factory #71

Uh oh!

Conversation

fbourigault commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To Do

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented May 12, 2017

Uh oh!

sagikazarmark commented May 12, 2017

Uh oh!

sagikazarmark commented May 12, 2017

Uh oh!

fbourigault commented May 12, 2017

Uh oh!

sagikazarmark commented May 12, 2017

Uh oh!

fbourigault commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbourigault commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbourigault commented May 16, 2017

Uh oh!

fbourigault commented Jun 15, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Jun 15, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Jun 15, 2017

Uh oh!

Nyholm commented Jul 1, 2017

Uh oh!

sagikazarmark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Jul 3, 2017

Uh oh!

Nyholm commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented May 11, 2017 •

edited

Loading

fbourigault commented May 12, 2017 •

edited

Loading

fbourigault commented May 12, 2017 •

edited

Loading